Support boolean values for --self-contained flag#52333
Support boolean values for --self-contained flag#52333Copilot wants to merge 13 commits intorelease/10.0.3xxfrom
Conversation
Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
|
I tested and it works correctly and the code looks right. There are a surprising number of tests that were dependent on this not working which is confusing. |
|
I tested all four possible options:
We will set _CommandLineDefinedSelfContained when false now though that seems correct (and we don't appear to use it anywhere). _SelfContainedWasSpecified will now also be true when setting false. |
|
note that before, --self-contained false was a weird grey area as it wouldn't set self-contained (so we were actually taking into account the boolean). This is because ForwardIfEnabled would only set self contained if the value were true (or empty). If false, it wouldn't forward the command from what I can tell. That means it's not as bad as I had assumed but it does mean if you have SelfContained specified elsewhere, the CLI false value won't affect it. |
|
@copilot there are merge conflicts with origin/release/10.0.3xx, can you please merge that in and fix the conflicts? |
…upport-self-contained-boolean # Conflicts: # src/Cli/Microsoft.DotNet.Cli.Definitions/Common/CommonOptions.cs
|
@baronfel review at your leisure. Not as bad as I originally thought but probably still worth taking. |
| Arity = ArgumentArity.ZeroOrOne | ||
| } | ||
| .ForwardIfEnabled([$"--property:SelfContained=true", "--property:_CommandLineDefinedSelfContained=true"]); | ||
| .ForwardAsMany(o => o.HasValue ? [$"--property:SelfContained={o.Value}", "--property:_CommandLineDefinedSelfContained=true"] : []); |
There was a problem hiding this comment.
IIRC nullable bool isn't quite the correct thing here - S.CL defaults will always provide a value for the bool, so HasValue will always be true. Here's a quick test bed I wrote:
> dotnet run .\scl.cs
Command: foo -o
Has option: True
Option value: True
Value type: explicit
Command: foo -o true
Has option: True
Option value: True
Value type: explicit
Command: foo -o false
Has option: True
Option value: False
Value type: explicit
Not using the nullable means that the changes to most of the other files don't have to happen.
Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
|
Retested and I can confirm that we get SelfContained false if not specified (and the other two _ are empty). If I set any self-contained CLI option, the other two _ options are true and SelfContained matches the boolean. @baronfel one weirdness is that SelfContained will be capitalized with this solution if set. Should we fix that to make sure it's lowercase always? |
|
fwiw, I also confirmed that |
|
Hmm, tests are failing because SelfContained is set where it wasn't before. I'll have to dig into those tests. |
|
@copilot |
Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
Fixed in commit 6ed90e0. The |
|
@copilot Can you ensure that when we set SelfContained, that the boolean value is lowercase as that's what msbuild expects? |
Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
Fixed in commit 91ddbe4. Changed the implementation to use a ternary operator |
Plan: Support boolean values for --self-contained flag
Analysis
The issue is that customers pass
--self-contained true/falsebut the CLI currently ignores thefalsevalue. TheForwardIfEnabledmethod only forwards when the boolean istrue.Changes Required
SelfContainedOptioninCommonOptions.csto useForwardAsManyinstead ofForwardIfEnabledto support both true and false valuesboolinstead ofbool?Implementation Details
SelfContainedOptionto forward both true and false values usingForwardAsManyNoSelfContainedOptionfor backward compatibility with--no-self-contained--self-containedand--no-self-containedoptions--self-contained=false→ framework-dependent (5 files)--self-contained=true→ self-contained (192 files)--self-contained(no value) → self-contained (192 files)--self-contained=true --no-self-contained→ error (as expected)bool?toboolto simplify implementation (commit f1a9e82)HasValuecheck and directly use the bool valueIfExplicitlyProvided()method was returning[]instead ofoption(commit 6ed90e0)Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.